-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Officer records assessment decision #264
Conversation
dec948e
to
27b4f7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very tidy work, thks! I've made a few small suggestions.
db/migrate/20240913140449_add_decision_enum_to_landing_application.rb
Outdated
Show resolved
Hide resolved
@@ -26,7 +26,7 @@ | |||
row.with_cell(text: application.landing_date) | |||
row.with_cell(text: application.departure_date) | |||
row.with_cell(text: application.application_submitted_at.to_date, html_attributes: { id: "submission-date" }) | |||
row.with_cell(text: '') | |||
application.application_decision ? row.with_cell(text: application.application_decision.capitalize, html_attributes: { id: "application-#{application.id}" }) : row.with_cell {govuk_button_link_to 'Make a decision', new_officer_landing_application_decision_path(landing_application_id: application.id), class: "govuk-!-margin-bottom-0", html_attributes: { id: "application-#{application.id}" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like a good candidate for a view component maybe? (Hurting my eyes to try to parse this long expression!)
end | ||
|
||
def and_i_should_see_my_decision | ||
page.find_by_id("application-#{@first_application_id}", text: "Approved") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think we ought to have a "flash message" of the "success" variety to reassure the assessor that the decision has been recorded?
I also find it doubly reassuring in situations where I'm redirected to an index page to have a confirmation of which item it is I've just operated on, e.g.
Application ID ABC123 has been rejected/accepted
application_decision = [:approved, :denied, nil].sample | ||
s.application_decision = application_decision | ||
if [:approved, :denied].include?(application_decision) | ||
s.application_decision_made_at = between_submission_and_landing_dates(landing_date, application_submitted_at) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for making this seeding realistic
also: I think our calls-to-action for the assessor ought to be a little less shouty, perhaps? |
27b4f7e
to
9e4fb86
Compare
9e4fb86
to
4f8873d
Compare
…User model We must specify `optional: true` because our landing applications will only have an assessor association once they have been assessed.
This will allow us to use the method in different test files.
This gives us additional query methods on the model: https://apidock.com/rails/ActiveRecord/Enum
This form object will allow us to validate the presence and value of an application decision in our controller method at the point that we want to update the model with the decision.
4f8873d
to
acabfb5
Compare
This will allow us to user `assert_template` in our test files, which was deprecated in Rails 5.0. It is nonetheless useful for us in allowing us to test conditional paths in our controller methods.
acabfb5
to
2e1633d
Compare
This is a complete new journey for our officer user, allowing them to approve or deny an application for landing. This involves changes at the data layer (associating our LandingApplication and User models), as well as an update to our application seeding to ensure applications have a decision (or no decision) assigned to them at random.
For now we don't have readable application IDs, so we are lacking a clear visual indication of the application being assessed. This will have to come in a future iteration.